Skip to content

update to vexflow beta#233

Closed
rvilarl wants to merge 1 commit intostringsync:masterfrom
rvilarl:vexflowBeta
Closed

update to vexflow beta#233
rvilarl wants to merge 1 commit intostringsync:masterfrom
rvilarl:vexflowBeta

Conversation

@rvilarl
Copy link
Collaborator

@rvilarl rvilarl commented Sep 16, 2024

@jaredjj3 please help me fixing the error.

  [RuntimeError] NoStave: No stave attached to instance.

      77 |   @util.memoize()
      78 |   getWidth(): number {
    > 79 |     return this.getVfKeySignature().getWidth() + KEY_SIGNATURE_PADDING;

We need to set the stave before calling getWidth but I do not know how to get access to the stave in keysignature.ts

/** Returns the width of the key signature. */
  @util.memoize()
  getWidth(): number {
    return this.getVfKeySignature().getWidth() + KEY_SIGNATURE_PADDING;
  }

@jaredjj3
Copy link
Collaborator

I'm on mobile, apologies for the lack of formatting. Hopefully this unblocks you.

Right now, vexflow.Stave objects are created downstream in rendering.Part.getStaves. Some possible options:

  • Create a temporary vexflow.Stave solely for measuring width.
  • Create vexflow.Stave objects in rendering.StaveSignature and pass it accordingly to rendering.KeySignature.

@rvilarl
Copy link
Collaborator Author

rvilarl commented Sep 16, 2024

Thanks! I am considering if it makes sense to have the stave. This is required to calculate the bounding box position correctly but not the height and width.

@jaredjj3
Copy link
Collaborator

In keysignature.ts, this seemed to fix it:

  private getVfKeySignature(): vexflow.KeySignature {
    const vfStave = new vexflow.Stave(0, 0, 0);
    const vfKeySignature = new vexflow.KeySignature(
      this.getKey(),
      this.previousKeySignature?.getKey() ?? undefined,
      this.getAlterations()
    ).setPosition(vexflow.StaveModifierPosition.BEGIN);
    vfKeySignature.addToStave(vfStave);
    return vfKeySignature;
  }

However, I've been having issues running the test via yarn test (Docker) and yarn jest. The test setup probably needs to be updated.

@jaredjj3
Copy link
Collaborator

I was able to run the tests. The change I mentioned does produce diffs, but they're negligible.

@rvilarl
Copy link
Collaborator Author

rvilarl commented Sep 23, 2024

I have set a PR in vexflow to avoid changes here.it should be allowed to get the width without assigning a Stave, I think.

@jaredjj3
Copy link
Collaborator

Thanks! I think that's a better solution since it will prevent other vexflow users from breaking.

@jaredjj3
Copy link
Collaborator

@rvilarl, I'm preparing to have this ready for VexFlow 5's release. Feel free to reach out to me when the issue we talked about has been addressed in VexFlow.

@jaredjj3 jaredjj3 closed this Dec 18, 2024
@jaredjj3
Copy link
Collaborator

jaredjj3 commented Jan 9, 2025

@rvilarl, just FYI, I did this work in #256.

@rvilarl
Copy link
Collaborator Author

rvilarl commented Jan 30, 2025

@ronyeh could you release a beta?

@rvilarl
Copy link
Collaborator Author

rvilarl commented Feb 25, 2025

@jaredjj3 this should be fixed in the vexflow 5.0.0-beta.1
your work around code should not be needed.

@jaredjj3
Copy link
Collaborator

@rvilarl removed in #275. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants